Skip to content

[SPARK-10180] [SQL] JDBC datasource are not processing EqualNullSafe filter#8743

Closed
HyukjinKwon wants to merge 9 commits into
apache:masterfrom
HyukjinKwon:SPARK-10180
Closed

[SPARK-10180] [SQL] JDBC datasource are not processing EqualNullSafe filter#8743
HyukjinKwon wants to merge 9 commits into
apache:masterfrom
HyukjinKwon:SPARK-10180

Conversation

@HyukjinKwon

Copy link
Copy Markdown
Member

This PR is followed by #8391.
Previous PR fixes JDBCRDD to support null-safe equality comparison for JDBC datasource. This PR fixes the problem that it can actually return null as a result of the comparison resulting error as using the value of that comparison.

@JoshRosen

Copy link
Copy Markdown
Contributor

Please update the pull request description to describe the changes in this patch.

@JoshRosen

Copy link
Copy Markdown
Contributor

Will this case be covered by the end-to-end docker tests?

@HyukjinKwon

Copy link
Copy Markdown
Member Author

Should I better write some test codes for this separately?

@yhuai

yhuai commented Nov 18, 2015

Copy link
Copy Markdown
Contributor

Can we add a test?

@HyukjinKwon

Copy link
Copy Markdown
Member Author

I added a simple test for this. I wanted to add a test including null but when the given value is null, Spark converts it to IsNull. To make this worse, more complicated logics cannot be applied here to test null-safety properly since now filters are pushed down only for basic operations (not even cast or arithmetic calculations). For now, I think this is the best to test this.

@HyukjinKwon

Copy link
Copy Markdown
Member Author

test this please

@HyukjinKwon

Copy link
Copy Markdown
Member Author

It looks Jenkins does not run the test for the past commits that I made as a user not added to whitelist. Would anybody run the test for this please if it looks good?

@yhuai

yhuai commented Nov 20, 2015

Copy link
Copy Markdown
Contributor

test this please

@SparkQA

SparkQA commented Nov 20, 2015

Copy link
Copy Markdown

Test build #46425 has finished for PR 8743 at commit bf0b22c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon

Copy link
Copy Markdown
Member Author

Just a question. Now it looks the PR for the end-to-end docker tests is merged. Do you think it needs all the tests for all the databases (namely MySQLIntegrationSuite and PostgresIntegrationSuite for now)?

@rxin

rxin commented Dec 30, 2015

Copy link
Copy Markdown
Contributor

@zsxwing can you review this one?

@maropu

maropu commented Dec 30, 2015

Copy link
Copy Markdown
Member

@HyukjinKwon +1, I think it'd better to add tests in MySQLIntegrationSuite and PostgresIntegrationSuite.

@HyukjinKwon

Copy link
Copy Markdown
Member Author

Hm.. actually I think adding tests more with the docker ingeration tests is a bit over-tested. It looks the comparison operators I used are all already being used in compileFilter and I think the logical operators can be covered in the existing test.

@maropu

maropu commented Dec 30, 2015

Copy link
Copy Markdown
Member

Yes and you're right. We need to clearly define the rule that decides which tests should be added in docker-integration-tests.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me several minutes to understand it. Maybe (attr IS NULL AND value IS NULL) OR (attr <> null AND value <> null AND attr = value) is easier.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, my previous expr doesn't work.

@zsxwing

zsxwing commented Dec 31, 2015

Copy link
Copy Markdown
Member

I'm wondering if it's okey to add sealed to Filter and remove case _. If so, the compiler can help us find such issue.

@HyukjinKwon

Copy link
Copy Markdown
Member Author

@zsxwing I will anyway resolve the conflicts first

@rxin

rxin commented Jan 1, 2016

Copy link
Copy Markdown
Contributor

Sorry I kept merging pull requests that made your life harder. Can you bring it up to date again? I promise this is the last one.

Conflicts:
	sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRDD.scala
@HyukjinKwon

Copy link
Copy Markdown
Member Author

@rxin Nothing is easy and happy new year:) I just resolved conflicts.

@HyukjinKwon

Copy link
Copy Markdown
Member Author

test this please

@SparkQA

SparkQA commented Jan 1, 2016

Copy link
Copy Markdown

Test build #2287 has finished for PR 8743 at commit 17b6953.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon

Copy link
Copy Markdown
Member Author

test this please

@SparkQA

SparkQA commented Jan 2, 2016

Copy link
Copy Markdown

Test build #2291 has finished for PR 8743 at commit a489a33.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Jan 2, 2016

Copy link
Copy Markdown

Test build #2294 has finished for PR 8743 at commit a489a33.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon

Copy link
Copy Markdown
Member Author

test this please

@SparkQA

SparkQA commented Jan 2, 2016

Copy link
Copy Markdown

Test build #2295 has finished for PR 8743 at commit a489a33.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@rxin

rxin commented Jan 2, 2016

Copy link
Copy Markdown
Contributor

Thanks - I've merged this.

@asfgit asfgit closed this in 94f7a12 Jan 2, 2016
zzcclp added a commit to zzcclp/spark that referenced this pull request Jul 27, 2016
@HyukjinKwon HyukjinKwon deleted the SPARK-10180 branch September 23, 2016 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants